perf(btree): decide page-reuse eligibility at free time, not per allocation - #27
Conversation
…cation allocate_page evaluated the in-session reuse rule by scanning the whole `freed` list and, for each entry, all of `free_page_consumed` — both plain Vecs, so `contains` is linear. Because `reuse_threshold` is the session-start `next_page_id`, every page freed during the txn sits below it and the cheap first disjunct is false for nearly every entry, so the full O(|freed| x |consumed|) scan ran on every single page allocation, with both vectors growing monotonically through the flush. The result is unbounded work rather than a deadlock: a server was found pinning one core at 99.4% for 84 minutes inside a single batch_write, 5048s of user CPU against 2.9s of system time and 104 read syscalls for the whole process lifetime, with 99.51% of perf samples in allocate_page. Both inputs to the eligibility decision are monotonic — reuse_threshold is fixed for the session and free_page_consumed only grows — so deciding once in free_page is equivalent to deciding per allocation. Eligible pages go to a second `freed_reusable` list that allocate_page pops in O(1). set_reuse_threshold re-partitions, for callers that set it after a free (normally a walk over two empty vectors), and drain_freed drains both lists so an eligible page no allocation claimed still reaches the deferred-free queue. Three tests cover the eligibility rule, post-hoc threshold setting, and a hang detector for the scan's return: with the old logic that test does not finish in 300s, with this change it completes in 0.00s.
There was a problem hiding this comment.
The equivalence argument holds. Checked what it rests on: reuse_threshold is set once per tree, always on a fresh BTree with empty lists; free_page_consumed is cleared before the trees open and taken after both drain_freed() calls, and allocate_page is its only pusher — so ineligible → eligible really is the only transition. Selection order is preserved (rposition → pop on a partition that keeps relative order), and nothing outside core.rs/drain_freed reads freed.
Three changes to land before this leaves draft.
1. Make the membership test O(1)
is_reusable_in_session still scans free_page_consumed linearly, and below-threshold frees dominate the flush profile this fixes — a CoW free of a pre-existing page is below the threshold by construction. So O(frees × |consumed|) survives, and |consumed| is bounded by the free-list window (WINDOW_PAGES × chain_capacity(page_size)), not by something small: a large enough flush is still a long stretch of pure CPU with no progress. Same failure shape, one order of magnitude out.
Change the shared sink to Arc<Mutex<FxHashSet<u64>>>. The commit path already collects it into a HashSet<u64> and only calls contains, begin_write only clears it, and nothing depends on its order or on duplicates — so this is near drop-in, makes the free-time check O(1), and removes the collect at commit.
Then seed consumed at window scale in allocation_cost_does_not_grow_with_the_freed_list. At its current size that test cannot see this term, so it would not catch a regression into it.
2. Make the wiring order unrepresentable instead of assumed
Eligibility is now decided at free time, so it is only correct if all three inputs are in place before the first free. Today they arrive through set_reuse_threshold, set_free_page_cache and set_free_page_consumed, callable in any order at any point in a session — and only the first of the three re-partitions. Wire set_free_page_consumed after a free and a cache-drawn page silently stays held back for the rest of the txn; that is a live gap in the new invariant, not a hypothetical, and an assert would only report it in debug builds while leaving the shape that allows it.
Replace all three setters with one, taking the session's allocation state as a unit:
pub struct PageSource {
pub reuse_threshold: u64,
pub cache: Arc<parking_lot::Mutex<Vec<u64>>>,
pub consumed: Arc<parking_lot::Mutex<FxHashSet<u64>>>,
}Pass Option<PageSource> at BTree::open — None for compaction's repack trees, which then have bump-only allocation by construction rather than by leaving a threshold at its default. Callers to update: the data and catalog trees in begin_write, and hist_tree in the catalog path (threshold 0, sharing the same cache and sink).
This buys three things beyond ordering safety: "cache wired but sink not" stops being representable, the re-partition loop in set_reuse_threshold is deleted rather than documented, and threshold_set_after_a_free_repartitions goes with it — it tests a path that should not exist. Net less code than this PR currently adds.
3. Drop the special case in is_reusable_in_session
fn is_reusable_in_session(&self, page_id: u64) -> bool {
page_id >= self.reuse_threshold
|| self.consumed_this_session(page_id)
}The reuse_threshold == 0 arm is unreachable by construction — page_id >= 0 always holds, so the zero-threshold case is already the first arm. Keep the reasoning in the doc comment; the branch itself only invites a reader to work out whether the two arms can disagree.
CHANGELOG
Drop the [Unreleased] block rather than moving it. 0.1.0 isn't released, so that section is still in progress and [Unreleased] above it implies it shipped. Folding it in is also wrong by that section's own rule — this scan was introduced and fixed inside the pre-0.1.0 window, so it's part of what 0.1.0 is, not a change to it.
Otherwise
Tests co-located and MemVfs-only, comments explain why, and "the first two pass unmodified against main" is the right evidence to offer. One note: the timing assertion is the only wall-clock one in src/ — keep the hang-detector framing in the docstring so it isn't later read as a benchmark and tightened.
…s O(1) The free-time eligibility test introduced with `freed_reusable` still scanned `free_page_consumed` linearly, and a copy-on-write free of a pre-existing page is below the reuse threshold by construction — so the dominant case in a flush kept paying O(frees x |consumed|), with |consumed| bounded by the free-list window rather than by anything small. Making the shared sink an FxHashSet drops that to O(1) per free: the commit path already collapsed it into a set and only tests membership, begin_write only clears it, and nothing reads its order. The hang detector now seeds `consumed` at window scale (WINDOW_PAGES x chain_capacity(page_size)) instead of a token 500 entries; at the old size it could not observe this term at all. set_reuse_threshold now debug_asserts that nothing has been freed yet, as do the two sibling setters, since eligibility is decided at free time and a threshold arriving afterwards would judge already-classified pages against the wrong one. That makes the previous re-partition branch unreachable, so it and the test covering it are gone rather than left as dead code. Also drops the `reuse_threshold == 0` disjunct, which never decided anything: every page id is >= 0.
|
Thanks — the free-time scan was a real hole, and you're right that it was the dominant case rather than an edge one. All of it addressed in The surviving The test could not see it, and now can.
Dead disjunct removed, with a one-line comment saying a zero threshold needs no special case since every page id clears it. CHANGELOG — Hang-detector framing kept, and the docstring now says outright that it is the only wall-clock assertion in Gates after the change: One thing I want to flag rather than leave implied: the end-to-end evidence I originally had — a real embedder flush completing on a large store — is gone. The 15 GB store that produced the profile hit an unrelated single-page AEAD failure on its next open, and the embedder's recovery path replaced it with an empty one before I could re-measure. So what backs this PR now is the profile, the two semantic tests passing unmodified against |
|
Followed up with a controlled A/B on real data, since the benchmark section only established "neutral at bench scale" and that is a weak claim for a page-allocator change. Method. Two identical bulk imports of 7,000 records through an embedder onto separate scratch stores, release build, same machine, same source database, no embedding provider configured (its network latency would dominate). The two builds differ only in
On With the branch the max stays in a band — 0.36, 0.88, 0.73, 0.90 s through the early windows and 3.7 s at 7,000 — and the whole import finished in about 40 minutes with zero errors, zero aborted flushes and no AEAD failures. The store reopened clean afterwards: healthy status, no AEAD or checksum lines, and the data readable. The diagnostic detail is that p50 is nearly identical in both arms (121 → 172 ms on Two things this does not show, which I would rather state than have you infer:
Also worth noting for anyone reading the commits: your own recent work on this path (allocation churn, Taking this out of draft on the strength of the above — the equivalence argument is confirmed, the free-time scan you caught is fixed in |
allocate_page/free_page decide reuse eligibility from three inputs (reuse_threshold, the shared free-page cache, and the consumed-page sink) that previously arrived through separate setters called on a freshly opened tree, with a debug_assert guarding against calling them out of order or after the first free. Collapsing them into a single PageSource, handed to BTree::open_session once at construction, makes a partially wired tree unconstructable instead of merely asserted against. The consumed-page sink itself becomes a ConsumedPages type: trees hold a ConsumedHandle that can record and test membership but never empty it, since emptying mid-session would strip the record that made already-banked pages eligible. Only the sink's owner, at a transaction boundary, can call take() to drain it. BTree::open remains for trees outside a write session (readers, compaction's repack trees), which still bump-allocate and may recycle anything they free.
|
Pushed Setters gone. A session's allocation state is one value, supplied at construction: BTree::open(pager, realm, root, next, page_size) // bump-only
BTree::open_session(pager, realm, root, next, page_size, source) // threshold + cache + sinkSo a late threshold and "cache wired but sink not" are both unconstructible. The asserts go with the setters — deleting the re-partition behind a debug-only check had left release builds with neither. The sink can no longer shrink mid-session. Free-time eligibility is only sound while Your Gates: 764 passed / 0 failed (764 vs 762 is the two new tests), same with |
2 participants
Opening as a draft: this touches in-session page reuse, which is load-bearing for crash safety, so I would rather have the equivalence argument below confirmed before this is treated as mergeable.
What
BTree::allocate_pagere-derived the in-session page-reuse rule on every call, scanning the wholefreedlist and, for each entry, all offree_page_consumed. Both areVec<u64>, so the membership test is linear:reuse_thresholdis the session-startnext_page_id(txn/write/txn.rs:115), so every page freed during a transaction is below it and the cheap first disjunct is false for essentially every entry. Each allocation therefore costs O(|freed| × |consumed|), with both vectors growing monotonically through the flush.This change decides eligibility once, in
free_page, and keeps the eligible pages in a secondfreed_reusablelist thatallocate_pagepops in O(1).Why it is equivalent, not merely faster
Both inputs to the decision are monotonic within a session:
reuse_thresholdis fixed once the transaction opens, andfree_page_consumedonly grows. A page can therefore only ever move from ineligible to eligible, and it can only be drawn from the shared cache before it is freed — so evaluating the rule at free time gives the same answer as evaluating it at allocation time.Two details that are easy to miss:
set_reuse_thresholdre-partitions, for callers that set the threshold after a free has already happened. Normally that is a walk over two empty vectors.drain_freeddrains both lists. An eligible page that no allocation claimed is still a page this session freed, and the deferred-free queue has to hear about it or it leaks.How this showed up
A long-running embedder was found pinning one core at 99.4% for 84 minutes inside a single
batch_write, with 5,048 s of user CPU against 2.9 s of system time and 104 read syscalls for the whole process lifetime — no I/O, no progress, and still answering health checks, because this is unbounded work rather than a deadlock.perf recordon that thread put 99.51% of samples inallocate_page, and the stack was:Testing
Three tests in
btree::tree::core::tests, all onMemVfs:reuse_eligibility_matches_the_threshold_rule— below-threshold held back, at/above recycled, and a cache-drawn page recyclable below the threshold.threshold_set_after_a_free_repartitions— the post-hocset_reuse_thresholdpath.allocation_cost_does_not_grow_with_the_freed_list— a hang detector rather than a benchmark: 20k allocations against 20k held-back frees, 10 s bound.The first two pass unmodified against
main, which is the evidence that this preserves existing semantics. The third does not finish in 180 s onmain(already compiled, so that is pure runtime) and completes in 0.00 s here.Gates run locally:
cargo nextest run --all-features763 passed / 0 failed, the same withPAGEDB_INVARIANT_CHECKS=1,cargo clippy --all-targets --all-features -- -D warnings,cargo deny check,RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features, andcargo fmt --all --check.Benchmarks
cargo bench --bench write_path, 3 runs per side on one machine. Neutral within measurement noise — run-to-run spread on this box reaches ±50% on several cases, which swamps every delta. The two tightest measurements (scaling_file_committed@100000, ±2% on both sides) agree to 0.4%.That is the expected result: at benchmark scale
freedandfree_page_consumedstay small, so the quadratic term never dominates. The gain is entirely at the tail, where a flush touches tens of thousands of pages — the regimeallocation_cost_does_not_grow_with_the_freed_listcovers, and wheremainstops completing at all. Anyone with a quieter machine will get a cleaner comparison than I could; I would rather state the measurement limit than quote a number I cannot reproduce.Tradeoff
One extra
Vec<u64>perBTree, and onecontainscheck moved from allocation time to free time. The free-time check is against the sameconsumedvector, so a pathological case would need a session that frees far more pages than it allocates — the inverse of the flush profile this fixes.On upgrade
No format, API, or durability-ordering change. Both lists are in-memory transaction state; nothing here is persisted, and an existing store is unaffected.